Add CEL rules to ResourceFlavor #1958

IrvingMg · 2024-04-09T06:33:13Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR replaces some of the validations executed by webhooks for the resourceflavor type with CRD validation rules.

Which issue(s) this PR fixes:

Relates to #463

Special notes for your reviewer:

Due to the resource constraints imposed by the API Server, only have been replaced those validations which costs don't exceed the limits.

Does this PR introduce a user-facing change?

Added CRD validation rules to ResourceFlavor.

ACTION REQUIRED: Requires Kubernetes 1.25 or newer

k8s-ci-robot · 2024-04-09T06:33:22Z

Hi @IrvingMg. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2024-04-09T06:33:30Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`a73a0c0`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6617ee7ef6492100080c2842

trasc · 2024-04-09T07:36:21Z

/assign

alculquicondor · 2024-04-09T14:52:50Z

/ok-to-test
/assign @trasc

kerthcet · 2024-04-10T08:02:55Z

apis/kueue/v1beta1/resourceflavor_types.go

@@ -78,6 +79,11 @@ type ResourceFlavorSpec struct {
 	// +optional
 	// +listType=atomic
 	// +kubebuilder:validation:MaxItems=8
+	// +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.key) ? x.operator == 'Exists' : true)", message="operator must be Exists when 'key' is empty, which means 'match all values and all keys'"


Just for discussion, from maintenance, do you guys think this is more simple comparing to the codes.

From maintenance POV it depends on how the rules look, From a runtime POV if we are able to fully drop the webhook for a type (like it was the case for local queue) then it's a win.

Make sense, apply the same policy through the whole project is important to avoid confusion, unless we can't fully remove the validation.

trasc

Generally LGTM, but unfortunately we cannot do to much for resource flavors.

pkg/webhooks/resourceflavor_webhook.go

pkg/webhooks/workload_webhook.go

pkg/webhooks/resourceflavor_webhook.go

apis/kueue/v1beta1/resourceflavor_types.go

test/integration/webhook/resourceflavor_test.go

trasc

/lgtm

Even if we ware not able to do too much about this.

@alculquicondor maybe for a followup, If we move the finalizer addition from the defaulter (and fully manage that in the controller) we can drop the mutation web-hook for this.

k8s-ci-robot · 2024-04-11T11:26:43Z

LGTM label has been added.

Git tree hash: 354007c50dbf3b3d2fc28d08bae7dbde2d433f29

apis/kueue/v1beta1/resourceflavor_types.go

alculquicondor · 2024-04-11T17:55:39Z

pkg/webhooks/resourceflavor_webhook.go

@@ -111,8 +111,6 @@ func validateNodeTaints(taints []corev1.Taint, fldPath *field.Path) field.ErrorL
 		if errs := validation.IsValidLabelValue(currTaint.Value); len(errs) != 0 {
 			allErrors = append(allErrors, field.Invalid(idxPath.Child("value"), currTaint.Value, strings.Join(errs, ";")))
 		}
-		// validate the taint effect
-		allErrors = append(allErrors, validateTaintEffect(&currTaint.Effect, false, idxPath.Child("effect"))...)

 		// validate if taint is unique by <key, effect>
 		if len(uniqueTaints[currTaint.Effect]) > 0 && uniqueTaints[currTaint.Effect].Has(currTaint.Key) {


I think we can replace uniqueness checks by using +listType=map and adding the appropriate keys to use.

But we can leave that for a follow up PR.

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2024-04-11T17:55:49Z

LGTM label has been added.

Git tree hash: 1a112b525742df57bc6e54b1afc3c99638fdbb95

k8s-ci-robot · 2024-04-11T17:55:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, IrvingMg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Add CEL rules to ResourceFlavor * Update comments * Add issue reference to TODO comment * Group similar test cases with DescribeTable * Update validation rule

Add CEL rules to ResourceFlavor

6516dc6

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Apr 9, 2024

k8s-ci-robot requested review from denkensk and kerthcet April 9, 2024 06:33

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 9, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2024

k8s-ci-robot assigned trasc Apr 9, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 9, 2024

kerthcet reviewed Apr 10, 2024

View reviewed changes

trasc reviewed Apr 10, 2024

View reviewed changes

pkg/webhooks/resourceflavor_webhook.go Outdated Show resolved Hide resolved

pkg/webhooks/resourceflavor_webhook.go Outdated Show resolved Hide resolved

pkg/webhooks/workload_webhook.go Outdated Show resolved Hide resolved

Update comments

139e9f3

alculquicondor reviewed Apr 10, 2024

View reviewed changes

pkg/webhooks/resourceflavor_webhook.go Outdated Show resolved Hide resolved

Add issue reference to TODO comment

06daa97

alculquicondor reviewed Apr 10, 2024

View reviewed changes

apis/kueue/v1beta1/resourceflavor_types.go Show resolved Hide resolved

test/integration/webhook/resourceflavor_test.go Outdated Show resolved Hide resolved

Group similar test cases with DescribeTable

aa9b099

trasc reviewed Apr 11, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2024

alculquicondor reviewed Apr 11, 2024

View reviewed changes

apis/kueue/v1beta1/resourceflavor_types.go Outdated Show resolved Hide resolved

Update validation rule

a73a0c0

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2024

k8s-ci-robot requested a review from trasc April 11, 2024 14:06

alculquicondor reviewed Apr 11, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor Apr 11, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2024

k8s-ci-robot merged commit aa2afc1 into kubernetes-sigs:main Apr 11, 2024
14 checks passed

k8s-ci-robot added this to the v0.7 milestone Apr 11, 2024

IrvingMg deleted the add-resourceflavor-cel-rules branch April 12, 2024 08:14

IrvingMg mentioned this pull request Apr 25, 2024

REQUEST: New membership for IrvingMg kubernetes/org#4907

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CEL rules to ResourceFlavor #1958

Add CEL rules to ResourceFlavor #1958

IrvingMg commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

netlify bot commented Apr 9, 2024 •

edited

Loading

trasc commented Apr 9, 2024

alculquicondor commented Apr 9, 2024

kerthcet Apr 10, 2024

trasc Apr 10, 2024

kerthcet Apr 11, 2024

trasc left a comment

trasc left a comment

k8s-ci-robot commented Apr 11, 2024

alculquicondor Apr 11, 2024

alculquicondor left a comment

k8s-ci-robot commented Apr 11, 2024

k8s-ci-robot commented Apr 11, 2024

Add CEL rules to ResourceFlavor #1958

Add CEL rules to ResourceFlavor #1958

Conversation

IrvingMg commented Apr 9, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Apr 9, 2024

netlify bot commented Apr 9, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

trasc commented Apr 9, 2024

alculquicondor commented Apr 9, 2024

kerthcet Apr 10, 2024

Choose a reason for hiding this comment

trasc Apr 10, 2024

Choose a reason for hiding this comment

kerthcet Apr 11, 2024

Choose a reason for hiding this comment

trasc left a comment

Choose a reason for hiding this comment

trasc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 11, 2024

alculquicondor Apr 11, 2024

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 11, 2024

k8s-ci-robot commented Apr 11, 2024

netlify bot commented Apr 9, 2024 •

edited

Loading